feat(chalice): Add span streaming support to Chalice integration#6503
feat(chalice): Add span streaming support to Chalice integration#6503ericapisani wants to merge 9 commits into
Conversation
Add span streaming support behind the trace_lifecycle=stream experiment flag. When enabled, start a segment span with Lambda/FaaS attributes for both HTTP view functions and event source handlers instead of setting a transaction name on the scope. Also adds aws_lambda to CLOUD_PLATFORM constants. Fixes PY-2312 Fixes #6010
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 208cf35. Configure here.
| invoked_arn = aws_context.invoked_function_arn | ||
| split_invoked_arn = invoked_arn.split(":") | ||
| aws_region = split_invoked_arn[3] if len(split_invoked_arn) > 3 else "unknown" |
There was a problem hiding this comment.
Although AWS sets this value and it's unlikely to be malformed, these extra guards were added to ensure that if it were, we don't crash the user's application
Codecov Results 📊✅ 89425 passed | ⏭️ 6013 skipped | Total: 95438 | Pass Rate: 93.7% | Execution Time: 307m 19s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 94.12%. Project has 2394 uncovered lines. Files with missing lines (1)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 89.80% 89.81% +0.01%
==========================================
Files 192 192 —
Lines 23460 23501 +41
Branches 8062 8076 +14
==========================================
+ Hits 21069 21107 +38
- Misses 2391 2394 +3
- Partials 1328 1334 +6Generated by Codecov Action |
| try: | ||
| return ChaliceEventSourceHandler.__call__(self, event, context) | ||
| except Exception: | ||
| exc_info = sys.exc_info() | ||
| event, hint = event_from_exception( | ||
| exc_info, | ||
| client_options=client.options, | ||
| mechanism={"type": "chalice", "handled": False}, | ||
|
|
||
| if has_span_streaming_enabled(client.options): | ||
| span = sentry_sdk.traces.start_span( | ||
| name=context.function_name, | ||
| parent_span=None, | ||
| attributes=_get_lambda_span_attributes(context), | ||
| ) | ||
| sentry_sdk.capture_event(event, hint=hint) | ||
| client.flush() | ||
| reraise(*exc_info) |
There was a problem hiding this comment.
I don't think we need to do anything here, just leave as is -- this code doesn't do any tracing so we don't need to change it
The set of spans we're creating should be the same as in the legacy path, we don't want any new spans when streaming
| span = sentry_sdk.traces.start_span( | ||
| name=aws_context.function_name, | ||
| parent_span=None, | ||
| attributes={ | ||
| **_get_lambda_span_attributes(aws_context), | ||
| **header_attrs, | ||
| **additional_attrs, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
This is also a new span compared to the legacy path -- please remove
As I understand, this integration is built on top of the AWS Lambda integration similar to e.g. Flask and WSGI? In that case, the pattern is usually that the parent integration creates the transaction/segment encompassing the whole request-response cycle, and the child integration then augments it with info specific to the child integration (e.g. better transaction name and source). The child integration may also create its own spans (e.g. for middlewares, DB queries etc.), but it shouldn't double-instrument the same thing as the top level segment
All of this is to say -- if we don't explicitly create a span in the legacy path, we shouldn't do it in span streaming either
There was a problem hiding this comment.
In this integration specifically, we seem to just want to augment whatever transaction is currently running with the same data as in the AWS Lambda integration.
The question for me is, does this integration even make sense standalone? Because if not, then we don't need to do any sort of post-enrichment here, as it'll already have happened in the AWS Lambda integration. Otherwise, we need to duplicate the enriching logic, but apply it to the current segment instead of creating a new span.
…led and where the Chalice integration is running on its own
Removes the creation of new spans but instead updates the segment of the current span should one be present (this happens if the AWS Lambda integration is also enabled and would be what creates the initial span).
… AWS lambda integration is not
| try: | ||
| return view_function(**function_args) | ||
| except Exception as exc: | ||
| if isinstance(exc, ChaliceViewError): | ||
| raise | ||
| exc_info = sys.exc_info() | ||
| if segment: | ||
| segment.status = SpanStatus.ERROR.value | ||
| sentry_event, hint = event_from_exception( | ||
| exc_info, |
There was a problem hiding this comment.
Bug: The Chalice integration fails to create a transaction with span streaming enabled if a segment doesn't already exist, resulting in lost transaction context for errors.
Severity: MEDIUM
Suggested Fix
Within the if has_span_streaming_enabled(client.options): block, add an else case to handle when type(current_span) is StreamedSpan is false. This new branch should create a transaction to ensure that transaction context is always established, mirroring the behavior of the non-streaming path.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: sentry_sdk/integrations/chalice.py#L120-L129
Potential issue: When span streaming is enabled in the Chalice integration, but no
preceding integration (like AWS Lambda) has created a segment, the system fails to
create a new transaction or span. This occurs because the code explicitly checks if
`type(current_span) is StreamedSpan`, and if this check fails (e.g., `current_span` is
`None` or `NoOpStreamedSpan`), there is no fallback logic to create a transaction.
Unlike the non-streaming path which sets a transaction name on the scope, this path does
nothing, causing any captured errors to lack transaction context.
There was a problem hiding this comment.
In this case, we don't want to create a new streamed span since no spans were created to begin with within the integration
| return app | ||
|
|
||
|
|
||
| def test_span_streaming_existing_span( |
There was a problem hiding this comment.
No test for span streaming when no outer Lambda segment exists
Both new streaming tests always wrap the request in an outer start_span, so the code path where has_span_streaming_enabled is True but type(current_span) is StreamedSpan is False (standalone Chalice with streaming, no AWS Lambda integration) is never exercised — in that path segment stays None, no span is created, and no transaction name is set, silently dropping all tracing data.
Evidence
- In
chalice.pylines 89-91, whenhas_span_streaming_enabledisTrueandtype(current_span) is StreamedSpanisFalse,segmentremainsNoneand the code falls through directly toview_function(**function_args)— no span is started andscope.set_transaction_nameis not called. - Both
test_span_streaming_existing_span(line 200) andtest_span_streaming_existing_span_error(line 224) always enter the request inside asentry_sdk.traces.start_span(...)block, soget_current_span()always returns aStreamedSpanand thesegment = Nonebranch is never hit. _make_span_streaming_appis only used by these two tests; no fixture or test exercises the streaming path without an enclosing segment.- The implementation silently discards tracing data in this case rather than creating a new segment span, contradicting the PR description ('start a segment span with Lambda/FaaS attributes for both HTTP view functions and event source handlers').
Also found at 1 additional location
sentry_sdk/integrations/chalice.py:130-137
Identified by Warden code-review, find-bugs · L2A-HR5
Add span streaming support behind the trace_lifecycle=stream experiment flag.
When enabled, start a segment span with Lambda/FaaS attributes for both
HTTP view functions and event source handlers instead of setting a
transaction name on the scope.
Also adds aws_lambda to CLOUD_PLATFORM constants.
Contains some duplication with #6498 since it's an AWS lambda framework.
Fixes PY-2312
Fixes #6010